Some high-level comments:

Good software engineering practices consider code as not just something for computers to run but also something for people to read. We can take the same attitude to pull requests: they are not just instructions to Git but also a narrative for people of what is being changed and how. Therefore it is a good idea to shape your PR intentionally, instead of just as a series of commits that you made along the way. For example, I like it that you added commit descriptions as needed. In this case there is a lot of overlap with the PR description, but in general that is not necessarily the case.

Now, different people have different opinions about how to structure a PR. Here is my take.

A well structured PR should be free of noise. Which means that each commit—and each line of change in each commit—must carry its own weight. In this PR, there are two merges from main. For the most part, such merges fail to carry their weight in a PR. Merges that apply cleanly are usually unnecessary. Merges that require manual intervention are sometimes necessary for the PR to be mergeable into main, but that can usually be left until the PR has been approved and ready for merge. They are not things that a PR reviewer needs to see.

It may be a good idea to include a merge commit in a PR review if the merge is particularly complicated. For example, a class that you use has undergone a significant restructuring. Even in that case, I would say that it is better to rebase your PR on top of the interfering change, so that the changes being reviewed closely resemble the code that will be merged.

Force push to a PR branch that is already undergoing review is a somewhat contentious topic. But I don't think there is general objection to force pushes when a PR is still in draft mode. So there shouldn't be any merge commits when you first mark a PR ready for review.

As for the remaining commits, I can accept that "Add CHANGELOG.md entry" carries its own weight. I don't think "Remove unused import" meets that bar. It really should have been folded into the previous commit.

What about "Minimize changes to the legacy query server client"? A well structured PR should present a straightforward and understandable narrative. Right now the narrative presented by the PR is "I added new code into the legacy query server client and then decided that I do not want them anymore". Is it important for PR reviewers to see that plot twist? Sometimes I do change or add code in one commit that is removed in a later commit. But that needs to be an intentional decision: for example, the intermediate state may help PR reviewers see how we get from the beginning to the end. That would make those extra lines of change carry their own weight. In this PR, though, I don't think it is useful for PR reviewers to see the legacy query server client code you added and then took out. They should just not be in the PR.

Would you like to try reworking the PR based on some of these suggestions? You can take the PR into draft mode and mark it as ready for review when you are done.

Very useful and actionable feedback! Thanks @cklin :) I'll move this PR back into Draft, and implement your suggestions.

I think we should hold back merging this PR until the CLI component has been released. If not, VSCode users will see a new command and discover that it doesn't seem to do anything.

I think we should hold back merging this PR until the CLI component has been released. If not, VSCode users will see a new command and discover that it doesn't seem to do anything.

If this is not really supported for older CLI versions, could we make this complete command only available if you are running a CLI version which supports this version? We do the same thing for the codeQL.quickEvalCount command, so I don't think it would be too difficult to add. You'd need to add a call the setContext command here, and then add a condition on the command in the package.json like so. You can then also add an additional check in the codeQL.trimCache handler like this.

Good idea @koesie10. I've been looking into it. I think a more scalable option is to extract the feature flags exposed by the CLI under codeql version --format=json as VSCode context keys. This would reduce boilerplate and hardcoding of CLI versions.

So I'll go ahead with this plan (independent steps):

• Introduce the feature flag in the CLI
• Decide on a naming convention for the context key (e.g. codeql.feature.<feature-name> or codeql.cli.<feature-name>), then:
  • In this PR: make the command conditional on the context key corresponding to the feature flag.
  • In this or another PR: extract all CLI feature flags as context keys.

Update: actually, to avoid snowballing this PR, I'll probably implement exactly what you suggest.

I think a more scalable option is to extract the feature flags exposed by the CLI ...

FYI

Within GitHub the phrase "feature flag" has a very specific meaning: https://thehub.github.com/epd/engineering/products-and-services/dotcom/features/feature-flags/

Even though we do not primarily work in dotcom (i.e., the github/github repo), we do use dotcom feature flags, for example to control the rollout of a CodeQL CLI release. So, to avoid confusion, it is best to use some other phrase (for example, "CLI feature") when we are not talking about the dotcom feature flag.

Note, I've left out the additional version check at the entry point of the trimCacheInDatabase method, my reasoning being that the lack of visibility as a user-accessible command is enough of a deterrent; in the unlikely case this gets triggered as part of some other code, I'll let the client do their own version check.